Skip to content

fix(store): enforce case-folded uniqueness for account identities - #757

Open
rodboev wants to merge 3 commits into
kenn-io:mainfrom
rodboev:pr/311-account-identity-address-key
Open

fix(store): enforce case-folded uniqueness for account identities#757
rodboev wants to merge 3 commits into
kenn-io:mainfrom
rodboev:pr/311-account-identity-address-key

Conversation

@rodboev

@rodboev rodboev commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

The account_identities primary key compares raw bytes while the application matches email-shaped identifiers case-insensitively, so two processes (serve plus a parallel CLI command, or two writers on PostgreSQL) can insert case-variant rows for one logical identity. The duplicate rows split the identity's signal set and make mergeSignalSet semantics break down, as described in #311.

This change persists the comparison-canonical form of each address and lets the schema enforce the invariant the application already assumed:

  • New address_key column holds NormalizeIdentifierForCompare(address): lowercased for email-shaped identifiers, verbatim for case-sensitive ones (Matrix MXIDs, phone numbers, handles). address remains the case-preserved display value. The normalization stays Go-owned; no SQL reimplements the email-shape heuristic.
  • A partial unique index on (source_id, address_key) WHERE address_key <> '' enforces one row per logical identity for every keyed writer on both backends. A concurrent case-variant insert now fails on the index and merges into the existing row through the writers' existing retry loop.
  • Store writers (AddAccountIdentity, batch confirmation, the legacy-config migration) look rows up by the key, so SQLite and PostgreSQL match under one rule instead of backend LOWER() variants.
  • Mixed-version safety: a previous-release binary keeps writing through the column's '' default, which the index exempts. On every store open, rows with missing keys are derived and case-variant duplicates merged under the identity-mutation lock: the earliest-confirmed row survives with its casing, signal sets union, and the identity revisions bump only when rows actually collapse.

Read-side behavior (attribution, activity, caches, dedup) is unchanged; those consumers keep reading address.

Closes #311

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (c687272)

Verdict: One medium-severity backward-compatibility issue requires attention.

Medium

  • internal/store/schema.sql:1790-1793, internal/store/schema_pg.sql:1803-1806: The new address_key column is NOT NULL without a default. Previous-release identity inserts omit this column, so older versions can open the migrated database but cannot add identities. Preserve compatibility with database-side derivation, or explicitly document this as a breaking storage-format change and update the downgrade contract.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 8m46s

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (918e081)

Verdict: High-risk migration race plus two Medium-severity compatibility issues require fixes.

High

  • Concurrent writes can be silently lost during migrationinternal/store/migrate_account_identity_key.go:40-47, 134-143
    The migration reads identities before installing the compatibility trigger. A previous-release PostgreSQL writer can insert during this window, and the rebuild may discard that identity. Acquire an exclusive table lock before reading/rebuilding or atomically coordinate the rebuild with legacy writers.

Medium

  • PostgreSQL downgrade compatibility differs from SQLiteinternal/store/dialect_pg.go:2049-2060
    The compatibility trigger derives address_key, but duplicate legacy inserts can still fail on the primary key, whereas SQLite suppresses them with INSERT OR IGNORE. Suppress existing duplicates in the PostgreSQL trigger or use conflict-safe legacy writes.

  • Unicode-normalized identities may not match downstream queriesinternal/store/identifier_match.go:25-40, internal/store/messages.go:1591-1598
    New keys use Unicode-aware Go normalization, while existing queries compare LOWER(address). SQLite’s ASCII-only LOWER can prevent attribution for non-ASCII addresses. Use persisted address_key values and matching normalization semantics.

  • Separate migrations can leave an incorrect trigger stateinternal/store/migrate_account_identity_key.go:163-172, internal/store/store.go:1311-1315
    Concurrent schema initialization can install the trigger, then another rebuild can remove it while the migration marker indicates success. Serialize migration checks under the same maintenance lock or make both operations atomic.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 9m22s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (a8e149b)

Verdict: One medium-severity identity-matching inconsistency remains; no critical or high-severity issues were found.

Medium

  • internal/store/messages.go:1105, internal/store/messages.go:1622, internal/store/message_identity_matches.go:80NormalizeIdentifierForCompare preserves case for dotless addresses, but participant and envelope matching lowercases candidates before comparing with address_key. Mixed-case dotless identities, including email-typed participant identifiers, may fail to resolve or mark matching messages as owned. Apply consistent shape-sensitive normalization to SQL and in-memory matching, with a regression test.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 15m9s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (fd11710)

Verdict: Changes require attention to two medium-severity attribution/cache issues.

Medium

  • internal/store/migrate_account_identity_key.go:168-175 — Cache revisions and existing message attribution are updated only when duplicate rows are merged. Archives with dotless identities may retain stale is_from_me values and analytics caches. Recompute attribution and invalidate both identity revisions whenever matching semantics change, even if row counts are unchanged.

  • internal/circleback/importer.go:1132-1134 — Configured meeting account emails and registered meeting sources still lowercase dotless identifiers, causing values such as Owner@localhost to be misclassified as inbound. Use store.NormalizeIdentifierForCompare consistently.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 14m56s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (0f35544)

Verdict: Two medium-severity compatibility issues require attention; no security regressions were identified.

Medium

  • cmd/msgvault/cmd/build_cache.go:1960 — Cache export unconditionally selects address_key, but the lower-level cache builder does not initialize schema migrations. Pre-migration archives may fail before migration runs. Initialize the schema before export or derive the key from address for legacy schemas.

  • internal/store/dialect_sqlite.go:1750 — The SQLite compatibility trigger depends on the connection-local msgvault_unicode_lower function. Older writers using the standard SQLite driver cannot provide it, causing inserts into migrated archives to fail. Make the trigger independent of connection-local UDFs or explicitly handle unsupported legacy connections.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 14m54s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (115c094)

Verdict: One medium-severity compatibility issue remains; no critical or high-severity issues found.

Medium

  • Unicode normalization mismatch in compatibility trigger
    • Location: internal/store/dialect_sqlite.go:1748-1754 and 1790-1795
    • SQLite’s built-in ASCII-only lower() differs from the Unicode-aware strings.ToLower used for current key generation. Legacy writes such as ÉMILIE@EXAMPLE.COM may produce inconsistent identity keys, causing duplicate identities or missed attribution.
    • Fix: Use the same Unicode-capable normalization as current writers and add a raw legacy-connection regression test with non-ASCII casing.

Reviewers: 2 done | Synthesis: codex, 13s | Total: 8m42s

@rodboev
rodboev marked this pull request as draft September 4, 2026 14:28
The account_identities primary key compares raw bytes while the
application matches email-shaped identifiers case-insensitively, so two
processes could insert case-variant rows for one logical identity and
split its signal set.

Persist the comparison-canonical form of each address in a new
address_key column (NormalizeIdentifierForCompare: lowercased for
email-shaped identifiers, verbatim otherwise) and enforce one row per
logical identity with a partial unique index on (source_id, address_key).
Store writers look rows up by the key, so both backends match under the
same Go-owned rule, and a concurrent case-variant insert fails on the
index and merges on retry.

Previous-release binaries keep writing through the column's '' default,
which the index exempts. Every store open derives missing keys and
merges any case-variant duplicates under the identity-mutation lock,
keeping the earliest confirmed_at, unioning signal sets, and bumping the
identity revisions when rows collapse. Non-email identifiers (Matrix
MXIDs, phone numbers, handles) keep byte-exact case-sensitive matching,
address remains the case-preserved display value, and read-side
attribution, cache, and query behavior is unchanged.

Closes kenn-io#311
The duplicate-collapse repair refreshes message attribution, which folds
identity matches into is_from_me. Running it before the provenance
migration on a pre-provenance archive let the backfill read those
identity-derived values as source-native and bake them into
source_is_from_me permanently. Move the repair after the provenance
migration; the ordering regression test fails at the earlier call site.
…ce timeout

The duplicate-collapse repair refreshes source-wide message attribution,
whose cost scales with archive size; under the pool-wide 30-second
PostgreSQL statement_timeout a large upgraded source would cancel the
repair and fail every subsequent open. Run it through runMaintenance
like the other archive-size-scaled migrations.

Create the partial unique index once per archive behind a migration
ledger entry, also inside the maintenance escape hatch, so a lock held
by a concurrent identity writer cannot trip the ordinary timeout during
open; IF NOT EXISTS covers a cancellation between the create and the
ledger write.
@rodboev
rodboev force-pushed the pr/311-account-identity-address-key branch from 115c094 to a3f11e8 Compare September 4, 2026 15:44
@rodboev rodboev changed the title fix(store): enforce account identity comparison keys fix(store): enforce case-folded uniqueness for account identities Sep 4, 2026
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (a3f11e8)

Verdict: No medium-or-higher severity issues found; the only reported issue is Low severity.

All reviewers agree there are no actionable Medium, High, or Critical findings.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 9m9s

@rodboev
rodboev marked this pull request as ready for review September 4, 2026 15:56
@rodboev
rodboev marked this pull request as draft September 5, 2026 02:59
@rodboev
rodboev marked this pull request as ready for review September 5, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

account_identities: schema PK is raw-bytes but compares are case-folded for emails

1 participant